Skip to content

fix: add storage non-mutation coverage to multisig_approval_init guar… - #425

Merged
godamongstmen897 merged 19 commits into
Goldii-locks:mainfrom
Yerimahjr:fix/353-harden-multisig-approval-init-guards
Sep 1, 2026
Merged

fix: add storage non-mutation coverage to multisig_approval_init guar…#425
godamongstmen897 merged 19 commits into
Goldii-locks:mainfrom
Yerimahjr:fix/353-harden-multisig-approval-init-guards

Conversation

@Yerimahjr

Copy link
Copy Markdown
Contributor

Closes #353

Summary

multisig_approval_init's authorization (require_admin) and precondition
(AlreadyInitialized) guards were already correctly implemented and ordered
before any storage read/write — verified by comparing against this repo's
own established pattern for this exact class of issue (prior closed
harden caller authorization and precondition guards in <function> issues
for admin_override_cancel_release and multisig_split_refund).

What was missing, per the issue's own acceptance criteria — "that no storage
entry is mutated in either case" — was test coverage proving it.

Changes

contracts/milestone-escrow/src/test.rs

Strengthened the two existing guard tests to additionally assert no storage
mutation on rejection:

  • test_multisig_approval_init_unauthorized_fails
  • test_multisig_approval_init_duplicate_fails (the "illegal source state"
    case — a second call to this one-time-init function)

Each now also confirms, after the expected typed error:

  1. is_multisig_approved(0).threshold is still the original value, not the
    attacker's attempted one.
  2. The attacker/impostor's address was never written into the signer set —
    confirmed by calling multisig_approve as them and getting Unauthorized.

contracts/milestone-escrow/src/admin_override_cancel_tests.rs

Unrelated pre-existing bug, fixed so the crate's test suite can compile and
run at all: this file (added by a separately-merged PR closing #383/#386)
was missing #![cfg(test)] — every other test module in this crate has it —
so it compiled unconditionally instead of only under cfg(test), making
soroban_sdk::testutils::Address::generate unavailable and the sibling
test module's setup_funded_escrow helper inaccessible. Added the missing
gate, the missing testutils::Address as _ import, and made
setup_funded_escrow pub(crate).

Test results

running 463 tests
test result: FAILED. 454 passed; 9 failed

The 454 passed include both target tests for this issue, and every other
test in the crate — this is effectively the crate's first-ever successful
compile+run, since it could not build before this branch. The 9 failures
are all pre-existing, in admin_override_cancel_tests::test_cancel_refund_*
(testing admin_override_cancel_refund, issue #386 — a different function
entirely), never having run before now. Root cause: admin_override_cancel_refund
calls admin.require_auth() redundantly before require_admin (which
already does so internally) — the same bug already fixed in the sibling
admin_override_cancel_release, just missed here. Flagging as a separate
issue rather than bundling an unrelated fix into this PR.

cargo fmt --check: not re-verified after this session's edits — please
confirm in CI.

Note on contracts/reports

This repo also contains a duplicate multisig_approval_init implementation
in contracts/reports/src/lib.rs, which has the same guards already correct.
However, contracts/reports is not a member of the workspace (Cargo.toml
workspace.members only lists contracts/milestone-escrow, and always has,
since the very first commit), and its package is literally named
milestone-escrow internally — a name collision that blocks simply adding
it as a workspace member. It cannot currently be built or tested at all, so
no changes were made there. Flagging for maintainers to decide whether it
should be wired into the workspace (under a different name) or removed.

Toyosi5566 and others added 7 commits August 26, 2026 19:30
…tests (Goldii-locks#290)

Harden cancel_escrow with two missing validation rules and add a full
test suite covering every guard, happy path, post-cancel state, event
structure, and milestone isolation.

Production changes (lib.rs):
- Add EmergencyPaused guard: cancel_escrow now returns Error::Paused when
  the contract is emergency-paused, consistent with all other user-facing
  endpoints
- Add duplicate-cancel guard: a second call while CancelLock is already
  active returns Error::EscrowLocked, preventing race conditions and
  redundant lock-sets

Tests added (test.rs) — 20 new tests:

  Invalid address guards:
    - test_cancel_escrow_zero_account_address_rejected
    - test_cancel_escrow_zero_contract_address_rejected

  Not-initialized guard:
    - test_cancel_escrow_not_initialized_fails

  Not-funded guard:
    - test_cancel_escrow_not_funded_fails

  Unauthorized guards:
    - test_cancel_escrow_stranger_unauthorized
    - test_cancel_escrow_arbiter_unauthorized
    - test_cancel_escrow_admin_unauthorized

  Emergency-paused guard:
    - test_cancel_escrow_while_paused_fails

  Duplicate-cancel guard:
    - test_cancel_escrow_duplicate_call_fails
    - test_cancel_escrow_freelancer_duplicate_after_client_fails

  Happy paths:
    - test_cancel_escrow_client_succeeds
    - test_cancel_escrow_freelancer_succeeds

  Post-cancel state validation:
    - test_cancel_escrow_blocks_fund
    - test_cancel_escrow_blocks_mark_delivered
    - test_cancel_escrow_blocks_approve_milestone
    - test_cancel_escrow_blocks_raise_dispute

  Event validation:
    - test_cancel_escrow_emits_exactly_one_event
    - test_cancel_escrow_event_contains_correct_caller

  Milestone state isolation:
    - test_cancel_escrow_does_not_mutate_milestones
    - test_cancel_escrow_all_milestones_released_still_succeeds

All 218 tests pass.
Guard the refund arithmetic so no input can cause a wrap or an unhandled
panic (issue Goldii-locks#395). Negative amount / released_amount and
released_amount > amount now return Error::InvalidAmount before any
arithmetic runs, complementing the existing checked_sub and remaining<=0
guards. The same guards are applied to the sibling
multisig_admin_override_release for consistency.

Adds a comprehensive suite to multisig_admin_override_refund_tests
asserting i128::MAX / i128::MIN operands return Error::InvalidAmount
(rather than panicking) and that valid amounts refund exactly
amount - released_amount, identical to prior behavior.

Closes Goldii-locks#395
…d tests (Goldii-locks#353)

Also fixes an unrelated pre-existing compile break in
admin_override_cancel_tests.rs (missing #![cfg(test)] gate and
inaccessible setup_funded_escrow helper) so the crate's test suite
can build and run at all.
@drips-wave

drips-wave Bot commented Aug 29, 2026

Copy link
Copy Markdown

@Yerimahjr Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Yerimahjr

Copy link
Copy Markdown
Contributor Author

Please review and merge

@Yerimahjr

Copy link
Copy Markdown
Contributor Author

Ci failure are caused by pre existing errors

Yerimahjr and others added 4 commits September 1, 2026 13:40
The arithmetic hardening is sound and merges cleanly. One of the new
tests failed:

  refund_valid_amount_equals_amount_minus_released_amount
  assertion `left == right` failed: left: 0, right: 1

The event is published -- multisig_admin_override_refund emits msadmref
on the success path, and the state and balance assertions above it all
passed, so the call had run to completion.

The tally was just read too late. env.events().all() reflects the most
recent contract invocation, and the test made three more
(is_multisig_locked, get_job, token.balance) between the override call
and the count. The sibling helper in multisig_split_refund_tests.rs
uses the identical idiom and passes because it reads the events first.

Moved the event assertion directly after the override call; the state
and balance assertions follow unchanged. No production code touched.

518 tests passing / WASM release build OK
…erride-refund-checked-arithmetic

fix(msadm): harden multisig_admin_override_refund arithmetic
@Yerimahjr

Copy link
Copy Markdown
Contributor Author

Conflict resolved

The storage change is sound: MilestoneTimeExtension moves from
persistent to temporary storage, which matches how the rest of this
contract already treats deadline-scoped state -- DeliveredAt is
temporary too, and extend_ttl is not used anywhere in the file, so no
new TTL obligation is introduced.

The branch also added `time_extension: 0` to a Milestone literal in
test.rs, but Milestone has only amount, released_amount, status and
delivered_at:

  error[E0560]: struct `Milestone` has no field named `time_extension`

Nothing else in the branch references such a field -- the extension is
keyed storage, not a struct member -- so the line was simply removed.

518 tests passing / WASM release build OK
…timize-storage-keys-footprint-for-milestone

fix: optimize milestone_time_extensions storage key footprint
…sed-milestone case

Two things kept this from compiling and passing.

The emergency-pause guard read DataKey::EmergencyPaused, which does not
exist:

  error[E0599]: no variant or associated item named `EmergencyPaused`
                found for enum `DataKey`

This contract has two separate pause flags -- DataKey::Ep for the
emergency pause (set by emergency_pause_admin_override, read by
ensure_not_paused) and DataKey::Paused for admin_pause_escrow. The
comment and the Error::Paused return both describe the emergency one,
so the guard now reads DataKey::Ep. The branch's own CancelLock check
is left as-is, since it returns EscrowLocked rather than the error
ensure_not_paused would give.

The new test test_cancel_escrow_all_milestones_released_still_succeeds
then failed. Its premise -- "no business rule blocks it" -- is no longer
true: cancel_escrow rejects a zero contract balance with InvalidAmount,
and releasing the only milestone empties the contract, so the balance
guard fired rather than anything to do with milestone status.

The test now mints 1 stroop back to the contract before cancelling, the
same workaround Goldii-locks#429 used for the same guard. That keeps it testing what
it claims -- that Released milestones do not themselves block a cancel --
instead of re-testing the balance guard.

538 tests passing / WASM release build OK
…ow-validation

feat(cancel_escrow): add business rule validations and comprehensive …
The storage non-mutation assertions are the point of this PR and are
kept in full: the rejected multisig_approval_init calls now also assert
the original threshold survives and the would-be signer was never
written.

The branch predates two tests that have since landed on main, and its
copy of test.rs reverts them:

  test_multisig_approve_unauthorized_fails
  test_multisig_approve_illegal_source_state_fails

Both cover approve-side guards -- an unregistered signer, and a
zero-balance source state -- and both assert the approval bitmap is left
untouched. Merging as-is took the suite from 538 to 536 while the PR
description said it was adding coverage, which is the kind of loss that
passes CI without complaint.

Restored verbatim from main. The remaining changes to
admin_override_cancel_tests.rs are rustfmt reflow plus a #![cfg(test)]
attribute, and were left as the branch had them.

538 tests passing / WASM release build OK
@godamongstmen897
godamongstmen897 merged commit 8b94a76 into Goldii-locks:main Sep 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden caller authorization and precondition guards in multisig_approval_init

4 participants